fix: return no-op for stale review_id 404 in dismiss_pull_request_review - #49745
Conversation
When `pulls.getReview` returns 404 for an explicit `review_id`, the
review is already dismissed or deleted (stale ID). Instead of hard-failing
the safe_outputs job, return `{success: true, skipped: true}` as a no-op,
mirroring the stale-thread pattern used in resolve_pr_review_thread.
Also adds a regression test covering the 404 path on explicit review_id.
Fixes: stale review ID hard-failing the safe_outputs job on PR Sous Chef
runs (run 30731801766).
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Handles stale pull-request review IDs as non-fatal no-ops, preventing unrelated safe outputs from failing.
Changes:
- Converts
getReview404 responses into successful skipped results. - Adds regression coverage ensuring dismissal is not attempted.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/dismiss_pull_request_review.cjs |
Handles stale review IDs safely. |
actions/setup/js/dismiss_pull_request_review.test.cjs |
Tests the explicit review-ID 404 path. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49745 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed: The fix correctly handles the 404 case at the right level (getReview), not by swallowing the error broadly — non-404 errors still re-throw.
- Pattern consistency: Mirrors the existing stale-thread no-op in
resolve_pr_review_thread.cjs. - Regression test included: A focused test covers the new path with the correct mock setup.
Minor Suggestion
- The test asserts
review_idbut omitspull_request_numberandrepo— full contract coverage would prevent silent shape regressions (see inline comment).
Positive Highlights
- ✅ Targeted try/catch scope — only the getReview call is wrapped, not the broader dismiss flow
- ✅ Non-404 errors are correctly re-thrown
- ✅
dismissReviewnot-called assertion is a good guard - ✅ Clear PR description with a design rationale note
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 18.8 AIC · ⌖ 7.95 AIC · ⊞ 7.1K
Comment /matt to run again
| expect(result.review_id).toBe(123); | ||
| expect(mockDismissReview).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The test doesn't assert pull_request_number or repo fields on the returned no-op — these are part of the documented return shape and missing assertions allow silent regressions if the structure changes.
💡 Suggested additions
expect(result.pull_request_number).toBeDefined();
expect(result.repo).toMatch(/\//);All fields of the contract surface should be covered so a future refactor can't silently drop them.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 6aa73e4. The 404 no-op regression now also asserts pull_request_number and repo in /home/runner/work/gh-aw/gh-aw/actions/setup/js/dismiss_pull_request_review.test.cjs so the full return shape stays covered.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
|
There was a problem hiding this comment.
The fix is correct and well-scoped. The targeted try/catch on pulls.getReview handles 404 gracefully by returning { success: true, skipped: true } without swallowing other errors, and the test confirms dismissReview is not called on stale review IDs.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.4 AIC · ⌖ 10.1 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: Approve with minor suggestions
Correct, well-scoped fix; no blocking issues found.
💡 Themes
- The 404-swallowing pattern mirrors the existing no-op approach in
resolve_pr_review_thread.cjsand theerror?.status === 404check matches established conventions used elsewhere in this codebase (check_permissions_utils.cjs,checkout_pr_branch.cjs), so it is not a fragile one-off. - Two non-blocking suggestions posted: (1) enrich the re-thrown non-404 error with review/PR context for easier log triage, and (2) add a negative test asserting non-404 errors are not swallowed, to guard against future regressions that could widen the 404 check.
- Correctly leaves
dismiss_pull_request_reviewout ofREPORT_ONLY_FAILURE_TYPES, keeping legitimate failures (e.g. author mismatch) fatal as intended.
🔎 Code quality review by PR Code Quality Reviewer · auto · 46.5 AIC · ⌖ 3.73 AIC · ⊞ 7.8K
Comment /review to run again
| repo: `${owner}/${repo}`, | ||
| }; | ||
| } | ||
| throw getReviewError; |
There was a problem hiding this comment.
Non-404 errors are re-thrown without added context, making them harder to diagnose in workflow logs when many reviews/PRs are processed.
💡 Details
When getReviewError.status is anything other than 404 (403, 500, rate-limit, network error), it propagates via throw getReviewError unchanged. The outer catch (error) at the bottom of main only records getErrorMessage(error), so the resulting failure message won't mention which review_id/pull_request_number the lookup was for. In a workflow that fans out over multiple dismiss requests, this makes triage harder.
Suggested fix:
} catch (getReviewError) {
if (getReviewError?.status === 404) {
return { success: true, skipped: true, reason: "review no longer exists", review_id: reviewId, pull_request_number: pullRequestNumber, repo: `${owner}/${repo}` };
}
getReviewError.message = `Failed to fetch review ${reviewId} on ${owner}/${repo}#${pullRequestNumber}: ${getReviewError.message}`;
throw getReviewError;
}Rationale: preserving the original error type/stack while enriching the message keeps the fatal-failure behavior intact but makes logs actionable.
There was a problem hiding this comment.
Fixed in 6aa73e4. /home/runner/work/gh-aw/gh-aw/actions/setup/js/dismiss_pull_request_review.cjs now enriches non-404 getReview failures with the review_id and owner/repo#pull_request_number context before rethrowing, so fatal safe-output logs stay actionable.
| expect(result.reason).toContain("review no longer exists"); | ||
| expect(result.review_id).toBe(123); | ||
| expect(mockDismissReview).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
No test asserts that non-404 errors from getReview (e.g. 500 or transient network failures) still propagate as failures rather than being swallowed.
💡 Details
The new test only exercises the happy path for the 404 no-op. There is no negative test verifying that a non-404 error (e.g. status: 500 or an error without a status field) still causes handler to return success: false (or throw, per the outer catch). Without this guard test, a future refactor that widens the getReviewError?.status === 404 check (e.g. to >= 400 or a truthy check) could silently convert real failures into false skipped: true no-ops, and CI wouldn't catch the regression.
Suggested addition:
it("still fails when getReview returns a non-404 error", async () => {
const serverError = Object.assign(new Error("Internal Server Error"), { status: 500 });
mockGetReview.mockRejectedValueOnce(serverError);
const result = await handler({
type: "dismiss_pull_request_review",
review_id: 123,
justification: "...",
});
expect(result.success).toBe(false);
expect(mockDismissReview).not.toHaveBeenCalled();
});There was a problem hiding this comment.
Fixed in 6aa73e4. I added a non-404 regression in /home/runner/work/gh-aw/gh-aw/actions/setup/js/dismiss_pull_request_review.test.cjs that keeps getReview 500s fatal and verifies dismissReview is not called.
|
@copilot Please give this PR a finishing pass and run the pr-finisher skill. This PR currently looks unblocked from the compact triage view, so please:
If everything is already complete, summarize readiness in the PR and proceed with any last small fixes.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Finished in
Hand-off: CI must be re-triggered by a maintainer before merge. |
|
@copilot Please give this PR a finishing pass and run the pr-finisher skill. This PR looks close to done from triage:
After that, run the pr-finisher skill.
|
Verified on
Ready for maintainers pending normal CI/check completion state on the current HEAD. |
|
@copilot Please give this PR a finishing pass and run the pr-finisher skill. Current triage signals:
After that, run the pr-finisher skill.
|
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "auto_merge",
"category": "bug",
"pr_number": 49745,
"risk": "low"
}
|
|
🎉 This pull request is included in a new release. Release: |
A
dismiss_pull_request_reviewmessage targeting a review that no longer exists causespulls.getReviewto 404, which propagates as a fatal error and fails the entiresafe_outputsjob — blocking unrelated safe outputs in the same run.Changes
dismiss_pull_request_review.cjs— wrap thepulls.getReviewcall in a targeted try/catch; on 404 return{ success: true, skipped: true, reason: "review no longer exists" }instead of surfacing a hard failure. Non-404 errors are re-thrown to the outer catch as before.dismiss_pull_request_review.test.cjs— regression test for the explicitreview_id+ 404 path: assertssuccess: true,skipped: true, and thatdismissReviewis never called.Design note
This mirrors the stale-thread no-op pattern in
resolve_pr_review_thread.cjs.dismiss_pull_request_reviewis intentionally not added toREPORT_ONLY_FAILURE_TYPES— legitimate failures (e.g. author mismatch) should remain fatal.